Skip to content

Support registry dependencies for bindeps #12062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

Support registry dependencies for bindeps #12062

wants to merge 8 commits into from

Conversation

@fee1-dead
Copy link
Member

This closes #10405.

Still need to add tests.

@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-cfg-expr Area: Platform cfg expressions A-crate-dependencies Area: [dependencies] of any kind A-dependency-resolution Area: dependency resolution and the resolver A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues A-registries Area: registries A-testing-cargo-itself Area: cargo's tests Command-fetch Command-fix Command-metadata Command-tree S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2023
@bors
Copy link
Contributor

bors commented May 3, 2023

☔ The latest upstream changes (presumably #12057) made this pull request unmergeable. Please resolve the merge conflicts.

@fee1-dead fee1-dead force-pushed the bindeps-registry branch from 49d0157 to 7a4a594 Compare May 4, 2023 11:10
@fee1-dead
Copy link
Member Author

cc @ehuss

I implemented this based on the things you have written for that issue. It's from over a year ago so things probably have changed. The most apparent issue with the current change is that we can't really try to download best effort in download_accessible and then populate the RustcTargetData since it will try to ask RustcTargetData for whether the target is enabled.

It could be possible to split RustcTargetData into two types: one is an early context which only has information about targets that are apparent (targets specified by cli) and then after downloading dependencies we add additional information to that to create the full RustcTargetData.

@fee1-dead
Copy link
Member Author

fee1-dead commented May 4, 2023 •
edited
Loading

Looks like I can pass a &mut RustcTargetData to collect_used_deps to make this work. But this is a really dirty hack though and I think that this part should be rewritten.

@ehuss
Copy link
Contributor

ehuss commented May 8, 2023

r? ehuss

@rustbot rustbot assigned ehuss and unassigned epage May 8, 2023
ehuss
ehuss reviewed May 8, 2023
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that it looks like download_accessible doesn't backtrack. That is, for example, you have the following scenario:

# foo/Cargo.toml
[dependencies]
shared = "1"
with-target = "1"

# shared/Cargo.toml
[dependencies]
bar = "1"
[target.'wasm32-unknown-unknown'.dependencies]
regex = "1"

# with-target/Cargo.toml
[dependencies]
shared = {version = "1", target = "wasm32-unknown-unknown"}

When it traverses foo, it sees shared and recurses into it. Target wasm32-unknown-unknown is not in requested_kinds or additional_kinds, so it does not download regex.

Then it recurses into with-target. It sees shared, recurses into it, but since shared is already in the used set, it is skipped. regex never gets downloaded.

Does it support that case?

src/cargo/ops/resolve.rs Outdated
has_dev_units,
requested_targets,
target_data,
&mut *target_data,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this reborrow is necessary.

Suggested change
&mut *target_data,
target_data,

src/cargo/core/package.rs Outdated
has_dev_units,
requested_kinds,
target_data,
&*target_data,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reborrow also doesn't seem to be necessary.

Suggested change
&*target_data,
target_data,

src/cargo/core/package.rs
Comment on lines +649 to +654
let activated = requested_kinds
.iter()
.chain(Some(&CompileKind::Host))
.chain(additional_kinds)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about chaining requested kinds + additional kinds. It seems like that could invite situations where it will download things it doesn't need. We've had some issues filed where cargo downloads too much (#11352), and it seems like this could make that worse.

Am I understanding correctly that this is creating a union of all possibilities, even if not all of them are used?

This seems to step on my fears of making download_accessible just as complicated as FeatureResolver in order to download the correct things, and I'm not sure how to balance that or if there are different approaches this can take.

src/cargo/ops/resolve.rs
.default_kind()
.into_iter()
.chain(pkg.manifest().forced_kind())
.chain(artifact_targets(pkg));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, what situations would need to merge artifact_targets here? My initial thought is that they should have all already been merged during download_accessible.

@ehuss ehuss mentioned this pull request May 8, 2023
Open
@ehuss
Copy link
Contributor

ehuss commented May 16, 2023

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2023
@fee1-dead
Copy link
Member Author

that was just a rebase. No work yet

@rustbot rustbot added the A-build-execution Area: anything dealing with executing the compiler label Jun 6, 2023
@fee1-dead fee1-dead closed this Jul 31, 2023
@fee1-dead fee1-dead mentioned this pull request Jul 31, 2023
Merged
bors added a commit that referenced this pull request Aug 25, 2023
@bors
Auto merge of #12421 - fee1-dead-contrib:bindeps-registry2, r=ehuss
e2c6a3e 
Support dependencies from registries for artifact dependencies, take 2

This is a continuation of #12062, and closes #10405.

r? `@ehuss`

Here are things this PR changes:

1. Add information about artifact dependencies to the index. This bumps index_v to 3 for people using bindeps.
2. Make `RustcTargetData` mutable for the feature resolver. This is so that we can query rustc for target info as late as resolving features, as that is when we really know if a package is really going to be used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ehuss ehuss ehuss left review comments

Assignees

@ehuss ehuss

Labels

A-build-execution Area: anything dealing with executing the compiler A-cfg-expr Area: Platform cfg expressions A-crate-dependencies Area: [dependencies] of any kind A-dependency-resolution Area: dependency resolution and the resolver A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues A-registries Area: registries A-testing-cargo-itself Area: cargo's tests Command-fetch Command-fix Command-metadata Command-tree S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bindeps: Support registry dependencies

5 participants

@fee1-dead @rustbot @bors @ehuss @epage